-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Thread notes #104
Thread notes #104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K UX - suhlasim, ze to je prilis prominentne miesto ak to je zbalene. A ak tam nieco je, tak by som ocakaval, ze to nebude zbalene ale bude to vzdy svietit.
Cize pridanie poznamky treba schovat za nejaky maly button a by default nezobrazovat nic.
@message_thread = message_thread | ||
@thread_tags_with_deletable_flag = thread_tags_with_deletable_flag | ||
@thread_messages = thread_messages | ||
@message_thread_note = message_thread_note |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toto vies vytiahnut z @message_thread
cize netreba explicitne posielat. sice to odpali select ale N+1 query problem nehrozi takze nevidim problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, cize ked nejde cez to iterovat, alebo nepouzivame ako kolekciu, tak si to neposielame. To je asi dobre pravidlo.
return unless @message_thread_note.last_updated_at | ||
|
||
@formatted_last_updated_at = l @message_thread_note.last_updated_at, format: :long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toto je best practice? nemali by taketo formatovacie veci byt vo view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neviem, ci je, resp. asi nie je. Zdalo sa mi to ako prilis komplikovany vyraz na to, aby som ho nechal vo viewe, najprv bol totiz tam. Este mozme pouzit jednu z tych verzii helpera, ktoru diskutujeme v inom vlakne. Si asi vyber.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No praveze taketo sa bezne do view dava
app/models/message_thread.rb
Outdated
|
||
thread.reload | ||
thread.destroy! | ||
thread.merge_identifiers.update_all(message_thread_id: target_thread.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zase kuzli linter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, este par krat mi to spravi, a vypnem ho. Zatial mam ale stale rad tie jeho obcasne navrhy na zlepsenia, co prinasa
app/models/message_thread_note.rb
Outdated
@@ -0,0 +1,15 @@ | |||
# == Schema Information | |||
# | |||
# Table name: message_threads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
create_table :message_thread_notes do |t| | ||
t.references :message_thread, null: false, foreign_key: true | ||
t.text :note | ||
t.datetime :last_updated_at, null: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pokial nemame usecase na specialny last_updated_at tak pouzivajme ten defaultny.
Ten UX necham ale chalanom doladit. Zatial som spravil aspon to, aby to bolo defaultne rozbalene, ked poznamka existuje. @jsuchal |
app/models/message_thread.rb
Outdated
return unless message_thread_note&.note | ||
|
||
if target_thread.message_thread_note | ||
join_string = "\nPripojené z vlákna #{title} #{I18n.l DateTime.now, format: :long}:\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toto takto nie, rozbije sa to na prekladoch. Ja by som tam dal len nejaky separator ---
a hotovo.
Otazky, resp. dilemy